Skip to content

Conversation

@pvizeli
Copy link
Contributor

@pvizeli pvizeli commented Mar 3, 2018

@codecov-io
Copy link

codecov-io commented Mar 3, 2018

Codecov Report

Merging #83 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #83   +/-   ##
=======================================
  Coverage   93.69%   93.69%           
=======================================
  Files          12       12           
  Lines         841      841           
=======================================
  Hits          788      788           
  Misses         53       53

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28cc671...b08a58f. Read the comment docs.

@mpdavis
Copy link
Owner

mpdavis commented Mar 6, 2018

lgtm

@mpdavis mpdavis merged commit c97f18a into mpdavis:master Mar 6, 2018
@zejn
Copy link
Collaborator

zejn commented Mar 6, 2018

Hi,

I don't think having a hard requirement on pycryptodome is ok, if you want to use pycrypto on GAE, because pycryptodome installs in the same namespace as pycrypto.

The only viable solution I currently see (if support for old GAE is still an option) is having a pure-Python RSA signing, with optional C backends, which the developers should choose themselves by specifying python-jose[cryptography] or python-jose[pycryptodome] or python[pycrypto]. Pycrypto and pycryptodome are mutually exclusive since they install in the same namespace, and cryptography might not be an option if installing on GAE.

@mpdavis
Copy link
Owner

mpdavis commented Mar 6, 2018

@zejn I believe you are correct.

I will prioritze your pure python RSA backend, and remove the requirement entirely, allowing for the override you describe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants